feat(relayenv): re-anchor mechanism#720
Open
aaron-zeisler wants to merge 20 commits into
Open
Conversation
Three tests (TestRealClient_CloseInvokesWrapperClose, TestRealClient_ReadsAfterCloseAreStillFunctional, TestRealClient_HandoverScenarioToday) exercise the real ld.LDClient against SSERelayDataStoreAdapter / streamUpdatesStoreWrapper. The T0 PoC could not validate the wrapper's Close behavior against the real client (it used a fake). These confirm the design's lifecycle caveat: ld.LDClient.Close() propagates through the wrapper to the underlying store. With store handover (next commit), the adapter — not the retiring client's wrapper — must own that lifecycle.
SSERelayDataStoreAdapter.Build now hands its existing wrapper to a second SDK client (re-anchor handover) instead of constructing a fresh wrapper around a fresh underlying store. The new anchor's client therefore reads populated, initialized data immediately — no empty-store window during re-anchor. The wrapper is refcounted: each handover bumps the count via acquire(); each client's Close decrements; only the final Close tears down the underlying store. This honors design §7's store-lifecycle caveat — Close must not tear down the shared store while another client still holds it — without leaking the store on final env teardown. Two PoC tests that asserted the old broken behavior (H1 in-memory sub-test, H5 in-memory wipe) are inverted to assert the post-fix invariant. The spike test TestRealClient_HandoverScenarioToday is renamed and rewritten to TestRealClient_HandoverPreservesUnderlyingStore covering the full refcounted lifecycle.
…mitAnchor Adds the API surface for the synchronous re-anchor sequence (design §7). - Reconcile no longer flips r.primarySdkKey when the anchor changes. Callers receive a ReconcileResult carrying an AnchorChange struct (PreviousAnchor, NewAnchor, NewAnchorPreviouslyAccepted) and drive the flip via the new CommitAnchor(key) method once they are ready. - ReconcileResult also exposes MobilePrimaryRepoint when the primary mobile key changed to a key that was already in the accepted set. In that case the key is not in additions and addCredential's gate won't fire, so the caller must invoke eventDispatcher.ReplaceCredential itself. - env_context's reconcileCredentials applies the anchor change immediately after Reconcile to preserve pre-T2.c behavior. The full synchronous sequence (build new client first, then commit) lands in the next commit. The MobilePrimaryRepoint case is handled now — this is the fix for the gap left by PR #712's gate (already-accepted mobile key becoming primary skipped event-dispatcher repointing). Two rotator tests that previously relied on Reconcile flipping the anchor in place now call CommitAnchor explicitly with the returned ReconcileResult.AnchorChange.
Implements the synchronous re-anchor sequence described in design §7. reconcileCredentials now interleaves add → re-anchor → remove and owns the new anchor's setup, replacing the prior commit's immediate CommitAnchor. The rotator strips the new anchor from additions in Case A so the async startSDKClient invocation in addCredential cannot race the synchronous client build. reanchor branches on whether a client already exists for the new anchor: Case A (no client): install peripherals if the key is brand new (envStreams, handlers, connection mapping); construct the SDK client synchronously via c.sdkClientFactory with store handover (Build returns the existing wrapper, so the new client reads populated initialized data); on err or !Initialized, roll back — do not CommitAnchor, log a structured error, leave the previous anchor authoritative; on success, install the client, rebuild the evaluator, then CommitAnchor + ReplaceCredential + big-segment stub. Case B (client exists, e.g. a former anchor in grace): reuse the client and skip the build; CommitAnchor + ReplaceCredential + big-segment stub. A new reconcileMu serializes concurrent reconciles without blocking other env operations (GetClient / GetStore / addCredential continue to run during the synchronous build). Rotator-test updates: tests that previously expected the anchor in the additions list now expect it stripped, and explicitly invoke CommitAnchor mirroring what env_context's reconcileCredentials does. T2.d (SDK-2543) will fill in reanchorBigSegmentSync; until then it is a no-op (matching today's behavior — no regression on re-anchor).
Regression coverage for the synchronous re-anchor sequence (design §7): - Case A success: re-anchor to a brand-new key builds a fresh client, hands over the store (same wrapper, still initialized, data intact), commits the anchor, and retains the old client during its grace period. - Case A init failure: a failing new-client build rolls back — anchor pointer stays on the old key, no broken client installed, old client preserved, initErr surfaced. - Case B: re-anchoring onto a key whose client still exists (a former anchor in its grace period) reuses the client and builds nothing new (clientCh stays empty), while the anchor flips. - Mobile-primary repoint: switching the primary mobile key to an already-accepted mobile key repoints the primary without spawning an SDK client — the gap PR #712's gate left, now driven via ReconcileResult.MobilePrimaryRepoint. All four pass under -race.
§7 previously described only Case A (re-anchor onto a brand-new key). Add the Case B path — re-anchoring onto a key that already has a live client (typically a former anchor still in its grace period): no Build, no store handover, just the flip + ReplaceCredential + big- segment re-wire that Case A also performs after init. The two paths converge after the flip; the caller branches on c.clients[newAnchor]. Also clarify failure handling (rollback is Case A only; structured Errorf log, no dedicated alarm infra), the Reconcile/additions stripping interaction, and update the consolidated T2.c/T2.d requirements table to reflect the case split and the synchronous ReplaceCredential call site.
gofmt the rotator's removeCredentialFromList helper, and reword the reanchorBigSegmentSync stub comment to avoid the godox TODO keyword (godox is enabled in .golangci.yml; run.tests is false so only production code is checked). No behavior change.
Re-applies the T2.c deferred-anchor-flip intent on top of the base's refactored Rotator/AcceptedSet API (primarySdkKey->anchorKey, set.primarySdkKey->set.anchor, new SDKKeyParams/MobileKeyParams builder, AcceptedKeys surface):
- rotator.go: Reconcile still returns ReconcileResult and defers the flip to CommitAnchor; removed the in-place anchor flip from reconcileSDKKeys (dropped its now-unused anchor param); Case A still strips the new anchor from additions. Fixed a stale comment referencing the removed legacy RotateWithGrace path.
- rotator_test.go: resolved onto the WithAnchor(SDKKeyParams{...}) builder API; updated TestAcceptedKeys and TestReconcileAlreadyExpiredKeyIsIgnoredOnAdd to the deferred-flip contract (CommitAnchor to observe the anchor; fresh anchor is stripped from additions).
- env_context_reanchor_synchronous_test.go: ported to the new builder API and AnchorKey() accessor.
- env_context_reanchor_test.go: inverted PoC H6/H7 to assert the T2.c fix (no nil swap-window; rollback keeps the old anchor authoritative), completing the same treatment H1/H5 received.
When a synchronous re-anchor's new client fails to initialize, reanchor rolls back and the environment keeps serving from the previous, still-valid anchor. It was also setting c.initErr to the new client's error — but the SDK returns ErrInitializationFailed for an invalid key, and the request middleware treats GetInitError() == ErrInitializationFailed as an env-level auth failure, returning 401 for every request. That rejected all traffic to an environment that was actually healthy, defeating the rollback at the HTTP layer. Leave initErr untouched on rollback (the env is healthy on the old anchor); the failure still surfaces via the existing structured Error log. Update the Case A init-failure and PoC H7 regression tests to assert GetInitError() stays nil and that the failure is logged at Error level.
The cleanup ticker path (cleanupExpiredCredentials -> triggerCredentialChanges) drained the rotator's StepTime queue and ran addCredential/removeCredential without holding reconcileMu, so a credential expiry firing during an in-flight synchronous re-anchor ran concurrently with the reanchor sequence. Both paths drain the same StepTime queue (the ticker could steal additions a reconcile just queued) and removeCredential could close a client mid-reanchor. The synchronous re-anchor widened this window versus the pre-re-anchor base. Take reconcileMu for the whole StepTime + add/remove pass in triggerCredentialChanges so the ticker is serialized against reconcileCredentials the same way concurrent reconciles already are. reconcileCredentials never calls that path, so there is no re-entrancy. Also gate re-anchor peripheral install solely on NewAnchorPreviouslyAccepted instead of on the Case A/B client-presence branch. The two signals always agree today (a client can only exist for a previously-accepted key), but branching peripheral setup on the rotator signal means a brand-new anchor is never stranded without its routing wiring even if that invariant were ever broken.
aaron-zeisler
commented
Jun 30, 2026
aaron-zeisler
commented
Jun 30, 2026
aaron-zeisler
commented
Jul 1, 2026
…rgon from code comments
Addresses PR review comments on comment quality:
- Drop references to the design doc (phase1-design.md, section numbers) and task/epic IDs (T2.x, SDK-25xx, M3) from code comments; those belong in the PR/commit history, not the source.
- Replace 'Case A'/'Case B' in comments with literal descriptions ('the anchor is a new key' / 'a previously-accepted key'), and simplify the re-anchor log strings to drop the same jargon.
- Clarify the reconcileMu comment (it serializes reconcileCredentials calls — only one runs at a time).
- Rephrase PR-number and PoC cross-references to keep the rationale without the identifiers, and fix two stale comments (a removed initErr behavior and the old primarySdkKey field name).
…nchorPeripherals addCredential and the re-anchor path both need to register a credential's downstream-facing routing — the env-stream registration, per-stream-provider HTTP handlers, and the connection->env mapping. That logic was duplicated: addCredential inlined it and reanchor had a near-verbatim copy in installAnchorPeripherals. Extract it into registerCredentialMappings(cred) (caller holds c.mu) and call it from both sites, removing installAnchorPeripherals. The name matches the codebase's existing 'credential mappings' vocabulary and replaces the ad-hoc 'peripherals' term throughout the comments. In addCredential the connection-mapping registration now runs with the other two mappings (before the anchor/mobile switch) instead of after it. This is not observable: the whole function holds c.mu, and the SDK client is started asynchronously (startSDKClient builds the client before taking c.mu), so it installs only after addCredential returns regardless of internal statement order; the three registrations touch disjoint subsystems with no ordering dependency.
Two behavior-preserving simplifications of the re-anchor path: - Extract buildAndCommitNewAnchorClient from reanchor: the ~45-line new-client build/rollback/install/commit block moves into its own method, leaving reanchor as a short four-way dispatch (register mappings, reuse existing client, offline commit, or build). - Extract rebuildEvaluatorLocked, shared by startSDKClient and the re-anchor build path, which previously had a near-verbatim copy of the evaluator construction each. Also clarifies two comments: the reanchor two-signal comment now explains that NewAnchorPreviouslyAccepted (were mappings already registered?) and existingClient (is there already a client?) answer distinct questions and legitimately differ when a previously-accepted non-anchor key is promoted; and restores the reconcileSDKKeys doc comment's opening line, which was dropped during the earlier catch-up merge.
Addresses findings from the multi-agent review of the re-anchor work: - Guard against a client/store leak when Close() races an in-flight re-anchor build: buildAndCommitNewAnchorClient now re-checks c.closed after re-acquiring c.mu and discards the freshly-built client instead of installing it into a torn-down env (Close does not hold reconcileMu, so it can interleave). Mirrors the existing guard in startSDKClient. - Clear initErr on all commit paths (reuse/offline as well as build) via commitReanchor, so a stale error from a prior client can't keep a healthy re-anchored env reporting failure to the request middleware. - store: mark a fully-closed streamUpdatesStoreWrapper and have acquire refuse it, so Build rebuilds a fresh store instead of resurrecting one whose underlying store was torn down. - Remove the reanchorBigSegmentSync no-op stub; the re-wire hook location is documented at the commit point in commitReanchor for the follow-up. - Rename rebuildEvaluatorLocked -> rebuildEvaluator (lock requirement stated in the doc); replace removeCredentialFromList with slices.DeleteFunc; drop the last stale 'peripherals' comment. Adds tests: offline re-anchor, previously-accepted non-anchor promoted to anchor, rotator-level AnchorChange/MobilePrimaryRepoint signalling, and a store rebuild-after-full-close regression.
A reconcile that both moved the anchor to a new key and immediately revoked the current anchor (no grace expiry) could, on a failed new-client build, still run the expiration phase and close the previous anchor's client — leaving GetClient() nil while the anchor pointer still named that key (503 despite the intended rollback), and leaving the rotator's accepted set naming the failed new key rather than the still-serving anchor. reanchor now reports whether it committed. On rollback reconcileCredentials backs out just the anchor change: it undoes the failed new anchor's credential mappings, calls the new Rotator.RevertAnchorChange to re-admit the previous anchor and drop the failed new key (realigning the accepted set with the un-moved pointer), and keeps the previous anchor's client serving by not expiring it. Other changes in the same payload still apply. RevertAnchorChange leaves a grace-demoted previous anchor (and its expiry) untouched. Adds an env-level rollback-with-immediate-revocation test and rotator-level RevertAnchorChange tests for both the revoked and grace-demoted cases.
…llback The re-anchor rollback backed out the failed new anchor by unconditionally calling removeCredential on it. For a previously-accepted key promoted to anchor (whose mappings predate the reconcile, so reanchor never re-registered them), that tore down its stream/handler/connection mappings while RevertAnchorChange kept it in the accepted set — leaving the rotator reporting a valid credential the env could no longer serve. Only undo the new anchor's mappings when it was brand new (NewAnchorPreviouslyAccepted == false), mirroring RevertAnchorChange's accepted-set logic so the env-side and rotator-side undos stay symmetric: a brand-new failed key is fully removed; a previously-accepted one reverts to the non-anchor key it already was, mappings intact. Adds a regression test (recording connection mapper) asserting a failed promotion of an already-accepted key keeps both its accepted status and its connection mapping.
RevertAnchorChange re-admitted change.PreviousAnchor whenever it was missing from the accepted set, without checking it was defined. When an env gains its first SDK key via a reconcile that then fails to build, the previous anchor is the empty (undefined) key, so rollback would insert "" into acceptedSDKKeys — an undefined credential the rotator otherwise never holds. Guard the re-admit on PreviousAnchor.Defined(). Adds a rotator test asserting a rolled-back first-SDK-key reconcile leaves no undefined key in the accepted set.
…nterleave commitReanchor released c.mu after installing the new client and then repointed the anchor/dispatcher, leaving a window where Close() (which does not take reconcileMu) could tear down clients and the dispatcher mid-commit — the commit would then flip to a closed client and call ReplaceCredential on a closed dispatcher. Split out commitReanchorLocked (caller holds c.mu) and hold a single continuous c.mu across install → commit on the build path; the reuse/offline paths call the self-locking commitReanchor, which now also bails if the env closed. Since Close() takes c.mu, the commit and teardown are now mutually exclusive. This mirrors addCredential, which already repoints event forwarding and reads rotator state under c.mu, so the locking discipline is unchanged. The SDK client is still built outside the lock, so GetClient/GetStore stay live during the build.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 40bfaba. Configure here.
aaron-zeisler
commented
Jul 1, 2026
reanchor now acquires c.mu once (defer Unlock) and holds it across the whole sequence — registering credential mappings and committing — releasing it only inside buildAndCommitNewAnchorClient around the SDK client build, which must not hold the lock so GetClient/GetStore stay live. This removes the self-locking commitReanchor wrapper (the commit helper now simply requires the caller to hold c.mu, no Locked suffix) and the separate lock acquisitions reanchor previously made for mapping registration and each commit path. Because the whole commit runs under one continuous c.mu hold, and Close() also takes c.mu, the commit and teardown are mutually exclusive — closing the remaining windows where Close could tear down the client or dispatcher mid-commit, and making reanchor's committed/rolled-back return value honest on the reuse/offline paths (it now reports the actual commit result, not an unconditional success).
aaron-zeisler
commented
Jul 1, 2026
buildAndCommitNewAnchorClient unlocked c.mu at its first line and re-locked it after the build — a helper releasing a lock its caller had taken, which read awkwardly. Split the pure client build into buildNewAnchorClient, which holds no lock (it touches only construction-fixed fields, like startSDKClient), and move the unlock/relock plus install back into reanchor so all of the lock management lives in one place. The build still runs outside c.mu: sdkClientFactory can block for up to sdkInitTimeout, and reconcileMu already lets GetClient/GetStore keep serving during construction. The reuse-existing-client and offline paths continue to skip the build entirely; behavior is unchanged.
This was referenced Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Implements T2.c — the re-anchor mechanism (SDK-2542) for concurrent SDK keys. When
sdkKey.valuechanges, relay now swaps its single upstream SDK client to the new anchor synchronously, preserving downstream connections and rolling back cleanly on failure.Jira: SDK-2542 · Epic: SDK-2453 · Milestone M3.
Targets
feat/concurrent-keys(not v8). Builds on the T0 PoC findings and the store-handover spike.Background
The design (
.agent-docs/concurrent-keys/phase1-design.md§7) and the T0 PoC established that re-anchoring is not a transparent side-effect of today's code: flipping the anchor pointer before the new client initializes leavesGetClient()nil mid-swap (PoC H6) and strands the env on init failure (H7); the in-memory store is rebuilt empty when the new client constructs (H1/H5). This PR closes those gaps.Changes (one commit per logical step)
test(store)— bring over the three store-handover real-client spike tests (one rewritten asTestRealClient_HandoverPreservesUnderlyingStore).feat(store)—SSERelayDataStoreAdapter.Buildreuses its existing wrapper on a second client construction (store handover → no empty-store window). The wrapper is refcounted: only the finalClosetears down the underlying store, so the retiring client'sClosecan't pull the store out from under the new anchor (design §7 lifecycle caveat).feat(credential)—Reconcileno longer flips the anchor; it returns aReconcileResult(AnchorChange+MobilePrimaryRepoint), and a newCommitAnchormoves the pointer. The new anchor is stripped fromadditionsin Case A soaddCredential's asyncstartSDKClientcan't race the synchronous build.feat(relayenv)— synchronous re-anchor with Case A (build new client + store handover + waitInitialized→ commit →ReplaceCredential→ big-segment stub; rollback on init failure) and Case B (reuse the existing client → commit →ReplaceCredential). Order: add → re-anchor → remove, serialized by a dedicatedreconcileMu.test(relayenv)— integration tests: Case A success, Case A init-failure rollback, Case B reuse (no new client), mobile-primary repoint.docs— document Case B in design §7.Acceptance criteria coverage
TestReanchorSync_CaseA_BuildsNewClientAndMovesAnchorTestReanchorSync_CaseA_InitFailureRollsBackTestReanchorSync_CaseB_ReusesExistingClientTestRealClient_HandoverPreservesUnderlyingStore,TestReanchorPoC_H5_StoreSurvivesReAnchor, H1 sub-testTestReanchorSync_MobilePrimaryRepoint_AlreadyAcceptedKey+ PR #712's gatePoC tests for H1 and H5 were inverted from "asserts broken behavior" to "asserts the fix" and retained as regressions.
Out of scope
reanchorBigSegmentSync) is defined as a no-op (matches today's behavior, no regression); T2.d fills it in.Post-merge note (catch-up onto
feat/concurrent-keys)The base moved 11 commits ahead after this branch was cut, including the
sdkKey→anchorrename (#722), the legacy single-key rotation path removal (#723), theSDKKeyParams/MobileKeyParamsbuilder +AcceptedKeys()reshaping (#729/#730), and the RAC/offline wiring toReconcileCredentials(#719). This branch now catch-up mergesfeat/concurrent-keysand re-applies the T2.c intent on top of the refactored API:Reconcilestill defers the anchor flip toCommitAnchor; the in-place flip that the rebasedreconcileSDKKeysintroduced was removed. Case A still strips the new anchor fromadditions.TestAcceptedKeys,TestReconcileAlreadyExpiredKeyIsIgnoredOnAdd) were updated to the deferred-flip contract (CommitAnchorto observe the anchor).ReconcileCredentials-based regressionTestReAnchoringToKeyStillInGraceReusesItsClientand this PR'sTestReanchorSync_CaseB_ReusesExistingClientboth cover Case B reuse; they are complementary (the former focuses on no-leak/close-of-revoked-key, the latter on no-new-build + anchor flip) and are both retained.Review fix — re-anchor rollback no longer poisons
initErrA review of the merge caught a regression in the rollback path: on a failed re-anchor to an invalid new key the SDK returns
ErrInitializationFailed, andreanchorwas storing that inc.initErr. The request middleware treatsGetInitError() == ErrInitializationFailedas an env-level auth failure and returns 401 for every request — which would have rejected all traffic to an environment that was still healthy on the previous anchor, defeating the rollback. Fixed:initErris left untouched on rollback (the env is healthy on the old anchor); the failure still surfaces via the structuredErrorflog. The Case A init-failure and H7 tests now assertGetInitError()stays nil and that the failure is logged.Review fix — serialize the credential cleanup ticker against re-anchor
A second review finding (MEDIUM): the cleanup ticker path (
cleanupExpiredCredentials→triggerCredentialChanges) drained the rotator'sStepTimequeue and ranaddCredential/removeCredentialwithout holdingreconcileMu, so a credential expiry firing during an in-flight synchronous re-anchor ran concurrently with the reanchor sequence. Both paths drain the sameStepTimequeue (the ticker could steal additions a reconcile just queued) and a tickerremoveCredentialcould close a client mid-reanchor. The synchronous re-anchor widened this window versus the pre-T2.c base. Fixed:triggerCredentialChangesnow takesreconcileMufor its wholeStepTime+ add/remove pass, serializing the ticker againstreconcileCredentialsthe same way concurrent reconciles already are (no re-entrancy —reconcileCredentialsnever calls that path). New regressionTestReanchorSync_CredentialExpiryDuringReanchorIsSerializedwedges a re-anchor open mid-build and asserts the ticker blocks until it releases.Review fix (LOW) — re-anchor peripheral install branches on one signal
reanchornow gates peripheral install (envStreams / handler / connection-mapper wiring) solely onchange.NewAnchorPreviouslyAcceptedrather than on the Case A/Bc.clients[newAnchor] != nilbranch. The two signals always agree today (a client can only exist for a previously-accepted key, sinceremoveCredentialdeletes the client in lockstep with the rotator dropping the key), but branching peripheral setup on the rotator signal means a brand-new anchor is never stranded without routing even if that invariant were ever broken.Note
High Risk
Changes core credential reconciliation, upstream SDK client lifecycle, and shared data-store refcounting during anchor rotation—failure or concurrency bugs could break serving or auth for whole environments.
Overview
Implements synchronous SDK anchor re-anchoring when
sdkKey.valuechanges: the upstream client swap no longer flips the anchor duringReconcileor races asyncstartSDKClient.Credential rotator now returns
ReconcileResult(AnchorChange,MobilePrimaryRepoint) and defers the anchor move toCommitAnchor; brand-new anchors are stripped fromadditionsso the env owns client build.RevertAnchorChangerealigns the accepted set on Case A rollback.reconcileCredentialsruns add → synchronousreanchor→ remove underreconcileMu(also held by the cleanup ticker’striggerCredentialChanges).reanchorbuilds or reuses the new anchor client before commit, callsReplaceCredentialon events/metrics, rolls back without poisoninginitErr, and handles mobile-primary repoint when the new primary was already accepted.Store handover:
SSERelayDataStoreAdapter.Buildreuses the existing wrapper with refcountedCloseso the retiring client does not tear down shared in-memory data.Design §7 documents Case A vs Case B; PoC tests assert fixed behavior; new integration and real-client tests cover success, rollback, reuse, serialization, and offline commit-without-build. Big-segment re-wire remains a stub (T2.d).
Reviewed by Cursor Bugbot for commit 2fc4f20. Bugbot is set up for automated code reviews on this repo. Configure here.